Skip to content

fix(studio): debounce flat slider drag commits to prevent rapid-fire writes#2190

Open
vanceingalls wants to merge 2 commits into
studio-flat-13-slider-dragfrom
07-10-fix_studio_debounce_flat_slider_drag_commits_to_prevent_rapid-fire_writes
Open

fix(studio): debounce flat slider drag commits to prevent rapid-fire writes#2190
vanceingalls wants to merge 2 commits into
studio-flat-13-slider-dragfrom
07-10-fix_studio_debounce_flat_slider_drag_commits_to_prevent_rapid-fire_writes

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

FlatSlider's drag support (added in the prior stacked PR) called onCommit synchronously on every pointermove/pointerdown, with no debouncing — unlike the legacy SliderControl, which coalesces rapid drag input into a single debounced commit. This stack rewires FlatSlider to match that established pattern:

  • Local draft state drives instant knob position / aria-valuenow feedback on every pointer event (no lag).
  • Actual onCommit calls are debounced 40ms during drag and flushed immediately on pointerup/release, deduped against the current committed value (skips a no-op commit).
  • Fixed a real bug found while wiring this up: onPointerUp previously read the draft closure, which can be stale if pointerdown+pointerup land in the same React batch (e.g. a very fast click) — it now recomputes the release value fresh from the event's own clientX.

This is a defensive fix for a reported issue ("live preview shows the wrong value while dragging; the correct value only appears after deselecting," reproduced by the reporter on Layer Blur). I was not able to reproduce the exact symptom myself despite extensive scripted browser testing, but the un-debounced commit-per-pointermove is a plausible root cause and this brings FlatSlider in line with the codebase's existing debounce pattern for exactly this class of bug.

Test plan

  • bunx vitest run — full studio suite passes (1585 passed)
  • bunx oxlint / bunx oxfmt --check on all touched files — clean
  • Please re-verify the original repro (Layer Blur slider drag on an image/video clip) live — this fix is well-reasoned but not confirmed against your exact repro

vanceingalls commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final current-head pass: d10ce38. Reviewed the stack boundaries and current diff against the flat-inspector contracts; required checks have no failures/pending checks and no unresolved review threads remain. No new drift found; residual notes are non-blocking.

@vanceingalls vanceingalls force-pushed the studio-flat-13-slider-drag branch from ad36891 to 102c503 Compare July 11, 2026 01:34
@vanceingalls vanceingalls force-pushed the 07-10-fix_studio_debounce_flat_slider_drag_commits_to_prevent_rapid-fire_writes branch from d10ce38 to 0e39019 Compare July 11, 2026 01:34

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.

@vanceingalls vanceingalls force-pushed the 07-10-fix_studio_debounce_flat_slider_drag_commits_to_prevent_rapid-fire_writes branch from 0e39019 to 6d0f755 Compare July 13, 2026 23:09
@vanceingalls vanceingalls force-pushed the studio-flat-13-slider-drag branch from 102c503 to 8b2b91c Compare July 13, 2026 23:09

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 6d0f7553c5ecda6c9a37d7ad60d8b364abc3a886. The throttled draft layer still has two direct races:

  • propertyPanelFlatPrimitives.tsx:262-265 blindly copies every parent value echo into local draft state, including while dragging, so a throttled intermediate commit can snap a newer pointer position backward.
  • Reset invokes onReset without cancelling the pending throttle/timer at :293-305,375-387; a delayed commit can overwrite the reset after the user sees it apply.

Gate prop-to-draft synchronization while actively dragging and cancel/flush the correct pending work before reset. Add fake-timer tests for mid-drag echoes and reset with a queued commit.

Verdict: REQUEST CHANGES
Reasoning: Throttled prop echoes can snap the control backward and a queued commit can overwrite reset.

— Deepwork

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive adversarial re-review at exact head . The previously reported prop-echo snapback and reset-vs-queued-commit races remain unchanged.\n\nStrength: The throttle correctly uses a leading commit plus a bounded trailing timer and deduplicates pointer-up against the last sent value ().\n\n**[blocker] A trailing slider callback can revert an intervening Grade edit.** The timer invokes the render-time closure (). Grade handlers spread the render-time whole object before replacing one field (). Queue Exposure, then change Contrast, apply a preset, or reset before 40 ms: the old Exposure callback commits its stale whole-object snapshot and silently undoes the newer action. Use latest-state/functional mutations or cancel/flush pending slider work at conflicting Grade actions; add a fake-timer cross-control regression.\n\n**[blocker] A rejected persist cannot restore the slider draft when the controlled value never changed.** The primitive owns and only resynchronizes when scalar changes (), while is fire-and-forget (). If persistence rejects and the parent value remains the original scalar, the effect does not rerun, leaving the knob and ARIA value at the failed attempted value while the document rolled back. The primitive needs an acknowledged revision/failure reset contract and a rejected-commit test.\n\n**[important] Unmount cleanup drops the latest queued edit.** Cleanup clears the timer without flushing (). Selection/accordion changes during a drag can discard the newest pointer value.\n\nVerdict: REQUEST CHANGES\nReasoning: The unchanged head still has the earlier slider races and adds cross-control stale-snapshot loss plus a visible rollback divergence on failed persistence.\n\n— Deepwork

@miguel-heygen miguel-heygen dismissed their stale review July 14, 2026 07:12

Formatting was corrupted during submission; replacing with corrected review.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive adversarial re-review at exact head 6d0f7553c5ecda6c9a37d7ad60d8b364abc3a886. The prior prop-echo snapback and reset-versus-queued-commit races remain unchanged.

Strength: The throttle uses a leading commit plus bounded trailing timer and deduplicates release (propertyPanelFlatPrimitives.tsx:245-305).

[blocker] A trailing slider callback can revert an intervening Grade edit. The timer invokes the render-time onCommit closure (propertyPanelFlatPrimitives.tsx:293-305), while Grade handlers spread the render-time whole grading object (propertyPanelFlatColorGradingSection.tsx:401-428). Queue Exposure, then change Contrast, apply a preset, or reset before 40 ms: the old callback commits its stale whole-object snapshot and undoes the newer action. Use latest-state mutations or cancel or flush pending work at conflicting actions; add a fake-timer cross-control test.

[blocker] A rejected persist cannot restore the slider draft when the controlled value never changed. draft only resynchronizes when scalar value changes (propertyPanelFlatPrimitives.tsx:251-265), while onCommit is fire-and-forget (:281-291). If persistence rejects and the parent value remains original, the effect does not run, leaving the knob and ARIA value at the failed value. Add an acknowledged revision or failure-reset contract and rejected-commit test.

[important] Unmount cleanup drops the latest queued edit. It clears the timer without flushing pendingRef.current (propertyPanelFlatPrimitives.tsx:266-270).

Verdict: REQUEST CHANGES
Reasoning: The unchanged head retains earlier slider races and adds stale cross-control loss plus visible rollback divergence on failed persistence.

— Deepwork

A pure debounce resets its timer on every pointermove, so a real drag
(events faster than 40ms apart) never commits until the pointer pauses
or lifts — killing live preview updates mid-drag. Throttle with a
leading-edge commit + trailing flush instead.
@vanceingalls vanceingalls force-pushed the 07-10-fix_studio_debounce_flat_slider_drag_commits_to_prevent_rapid-fire_writes branch from 6d0f755 to 98a23fe Compare July 14, 2026 08:07
@vanceingalls vanceingalls force-pushed the studio-flat-13-slider-drag branch from 8b2b91c to 7a2ccdc Compare July 14, 2026 08:07
@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Two of three fixed in PR #2416, commit 267cdfce1; one acknowledged but deferred.

Fixed — trailing commit can revert an intervening edit: FlatSlider now reads onCommit through a ref (onCommitRef, reassigned unconditionally every render) instead of closing over it at scheduleCommit time. Grade's per-detail onCommit closures spread the render-time whole grading object, so a queued Exposure commit that used to fire ~40ms later with a stale snapshot now resolves against whatever the latest render's closure captured — including a Contrast/preset/reset committed in between. Regression test: a trailing throttled commit uses the current render's onCommit, not the one captured when it was scheduled.

Fixed — unmount drops the latest queued edit: the unmount cleanup now flushes pendingRef.current through onCommitRef.current before clearing the timer, instead of silently discarding it. Regression test: flushes a still-queued trailing commit on unmount instead of dropping it.

Deferred — rejected persist can't restore the draft: this needs an actual failure signal threaded back from onCommit (currently (next: number) => void, fire-and-forget, with no rejection/acknowledgment channel), and that contract change touches every FlatSlider caller across Grade/Style/Motion — Style and Motion's commits already resolve/error-handle at a different layer than Grade's, so this isn't a one-line fix I want to rush without checking each caller's actual failure behavior. Filing this as a follow-up rather than guessing at a fix here — happy to scope it as its own PR if you want it prioritized.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 98a23fe7b341610d7598b38735f60d9d5c335776.

Both incremental commits are byte-equivalent to the previously reviewed head (patch-id 553da8c5…). propertyPanelFlatPrimitives.tsx:251-342 still overwrites drag-local state on controlled prop echoes, clears rather than flushes pending work on unmount, and leaves reset/trailing callback races. Grade callers still close over whole-object render snapshots, so a queued slider commit can revert an intervening sibling edit. A rejected persist can also leave the draft at the attempted value when the controlled value never changes.

No requested fake-timer, rejected-persist, or unmount regression test was added.

Verdict: REQUEST CHANGES
Reasoning: The force-push is patch-equivalent; the stale-snapshot, rollback, reset, and dropped-pending correctness blockers all remain.

— Deepwork

@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Confirming: this PR's own head is unchanged, correctly — the fixes for this review's findings were not squashed backward into it. They land in PR #2225 and PR #2416, which sit directly on top of this PR in the same Graphite stack (#2120 → ... → #2190 → #2225 → #2369 → #2416).

This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite (gt merge / sequential merges), not left partially merged with #2225/#2416 dangling for some indefinite period. Main never sits on this PR's head as a final state — by the time this PR merges, the rest of the stack merges with it in the same pass, landing the fixes immediately after.

Evaluating each intermediate head against "is this correct if it were the final merged state" doesn't match how the stack actually ships. Could you review mergeability at the stack tip (#2416, which contains every fix) rather than blocking each intermediate PR on findings that are already resolved a few commits further up the same stack? Happy to walk through the specific commit that fixes any finding you want re-verified at the tip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants